-
Notifications
You must be signed in to change notification settings - Fork 3
Fixed parsing of json maps #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly identifies and fixes a bug where YAML values containing JSON maps were not parsed correctly. The approach of using json_decode is appropriate. However, the implementation has a critical compatibility issue by using str_starts_with (a PHP 8+ function), and it could be made more robust and efficient. I've provided a detailed comment with a suggested code change to address these points, ensuring backward compatibility and improving the code's resilience.
| if ( str_starts_with( $value, '[{' ) || str_starts_with( $value, '{"' ) ) { | ||
| $decoded = json_decode( preg_replace( '/^.*?:\s*/', '', $line ), true ); | ||
| if ($decoded !== null || (json_last_error() === JSON_ERROR_NONE && $decoded === null)) { | ||
| // Accept null as valid JSON value, but only if no error occurred | ||
| $value = $decoded; | ||
| } | ||
| // Otherwise, leave $value as-is (fallback to original string) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good fix for handling JSON values. However, the current implementation has a few issues that should be addressed:
- PHP Compatibility (
critical): The use ofstr_starts_with()was introduced in PHP 8.0. Using it will break backward compatibility for projects running on older PHP versions, which this library appears to support. - Redundant Parsing (
high): The code usespreg_replace('/^.*?:\s*/', '', $line)to re-extract the value from the line. This is unnecessary and less efficient, as the value has already been correctly parsed into the$valuevariable. - Brittle JSON Detection (
medium): The condition to detect a JSON string is too specific. It will fail for valid JSON with leading whitespace (e.g.,{ "key": "value" }) or other valid JSON structures.
I've provided a suggestion below that resolves all these points by using a more robust and backward-compatible approach.
// The default inline parser (_toType) does not correctly handle complex JSON objects.
// Attempt to parse as JSON first for values that look like potential JSON.
$first_char = isset($value[0]) ? $value[0] : '';
if ($first_char === '{' || $first_char === '[') {
$decoded = json_decode($value, true);
// If the value is valid JSON, use the decoded array.
// This correctly handles valid JSON 'null' as well.
if (json_last_error() === JSON_ERROR_NONE) {
$value = $decoded;
}
// Otherwise, leave $value as a string for the legacy parser to handle.
}
Valid YAML such as the following is not correctly parsed by the Spyc fork used in wp-cli. If you validate the following with other YAML validators such as yamllint.com, it is returned as valid Yaml.
This should be represented in PHP as:
However, when running through the Spyc.php implementation, the following is returned:
This can be tested by running one of the example scripts
I have added two additional tests in the
spyc.yamlfile to test this issue. If you run the exampleyaml-load.phpscript you will see that they are handled correctly and all of the existing examples are unchanged.